Add support for wasi-http #56
Conversation
There was a problem hiding this comment.
Thanks for the contribution!
It's great to have the change somewhat self-contained in the imports/wasi_http, a few things I'd love to see added:
-
I know we didn't set a great precedent with
imports/wasi_snapshot_preview1, tho adding a README toimports/wasi_httpwould be great: I'd love to have things like a short description of what the module does, maybe a code example of how to use it, and links that will be relevant to help us maintain the code (e.g. locations where this module is implemented for other runtimes, related discussions, specification drafts, etc...) -
Having a guest program to test the module, we keep a few of those in https://github.com/stealthrocket/wasi-go/tree/main/testdata; this would be extremely important to ensure that we don't introduce regressions when we will make updates to this code. I know WASI makes constructing client/server applications challenging, this gist may be a good starting point to put together a test program using standard WASI features https://gist.github.com/chriso/6c71e968ef1002981a6ff46ceaa39ee3
|
Ok, since this looks good, I will push it forward to something that might actually be mergeable (will likely take a few days because of real life) Will also add the README.md and a test. |
|
@achille-roussel this is now ready for review. I added a simple integration test. The coverage isn't great, but I plan to expand it in future PRs. Let me know if you want me to expand it before this merges. Thanks! |
brendandburns
changed the title
There was a problem hiding this comment.
This is looking good!
I left a question about the .o file that was checked in, will merge when you get back to us @brendandburns
Thanks for the contribution 🙌
There was a problem hiding this comment.
This file looks like an intermediary compilation object, do you know if it needs to be checked in?
There was a problem hiding this comment.
This is actually a by-product of running wasi-bindgen
If you'd prefer, I can set up the Makefile to download that binary and re-generate those artifacts if you want.
testdata/c/hello_world.wasm
Outdated
There was a problem hiding this comment.
Do you happen to have the sources for this file?
If not, I imagine we can reproduce it pretty easily, I can handle that post-merge if needed.
There was a problem hiding this comment.
This file was already checked in:
https://github.com/stealthrocket/wasi-go/blob/main/testdata/c/hello_world.wasm
I'm not sure if that is on purpose. I suspect that it changed because I was using a different version of the clang compiler than when that test was checked in when I re-ran the tests.
If you want, I can remove it or revert my changes, let me know.
There was a problem hiding this comment.
Ah all good, this isn't a big deal if it's just a compiler version thing, I missed that we had the source checked in already 👍
|
Thanks for the review. I can squash commits if you want before it merges (or you can squash on merge) |
|
I tagged a |
@achille-roussel as discussed in tetratelabs/wazero#1519 this adds wasi-http support.
This code is very incomplete and hacky, but it is working for limited use cases.
If this integration looks correct-ish, I will polish this up, add tests etc.
Thanks!